Repository navigation
Reuse a C# project's in-memory PE reference while its semantic version holds - #20460
xperiandri wants to merge 14 commits into
Conversation
✅ Release-note check exemptedThe NO_RELEASE_NOTES label or localization-bot authorship exempts this pull request. |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xperiandri
left a comment
There was a problem hiding this comment.
Review from the architecture side. The direction is right, and it has a precedent worth citing in the description: Roslyn's own cross-language reference cache, SolutionCompilationState.SkeletonReferenceCache, keys the metadata-only skeleton of a project on Project.GetDependentSemanticVersionAsync and, for an unchanged version, returns what it has "regardless of whether it succeeded or not". The editor already treats that version as "the reference did not change" in WorkspaceExtensions.getReferencedProjectVersions (the snapshot-reuse guard), so this PR makes the PE-reference stamp consistent with that guard instead of racing ahead of it.
Findings, most important first. Each has an inline suggestion; the first three go together.
- A cache hit still awaits
GetCompilationAsync. On a hit the compilation only feedsRefresh, and after the first emit result nobody reads it again — but when Roslyn has dropped the final compilation under memory pressure the call rebuilds it and the entry pins it: one full compilation per referenced C# project per options recompute, in exactly the scenario the PR targets. Roslyn's cache checks the version first and touches the compilation only on a miss. RefreshracesEmitted()(reactor thread vs. FCS emit thread, no shared lock) and can re-pin a compilation after its emit succeeded;TryGetCompilationalso ignorespinned. With (1),Refreshdisappears and the entry becomes single-writer.and!would be the first use in the repository (FSharp.instructions.md: no foothold) and buys nothing here. Gone with (1).- The new
peReferences.TryRemoveinClearOptionsis dead code — it sits in the arm that is entered for F# project ids only, so a removed C# project keeps its entry until the next sweep. TheClearSingleFileOptionsCachetwin is dead for the same reason (miscellaneous-files project), and so are the pre-existinglastSuccessfulCompilationslines next to them. - Nits: three
// Stop strongly holding…comments restateEmitted(); the comment abovepeReferencesis now covered by the type summary; the release note carries the war story.
Tests (the checklist asks where this could be covered): the editor test host cannot hold a C# project today — TestHostWorkspaceServices.GetLanguageServices in RoslynHelpers.fs throws for anything but F#, so SupportsCompilation is never true there. Two routes: (a) compose Microsoft.CodeAnalysis.CSharp.Workspaces into the test host and assert that TryGetOptionsByProject hands back the same ReferencedProjects.[0] object across a body-only edit of the C# project and a different one after a declaration edit; (b) cheaper — make the version/pin logic an internal type with the emit function injected and test it directly: same object for the same version, new object on a version change, pin dropped after a result, pin kept and stamp bumped on cancellation. I checked the suggested blocks with Fantomas and with a stand-in script covering the cases in (b); a full FSharp.Editor build was not run.
Not for this PR, but worth knowing: with the transparent compiler off, Stamp = hash(GetDependentVersionAsync) still creates a new IncrementalBuilder — which imports its non-framework references afresh — on every edit in a referenced C# project, body-level included, so the legacy path does not benefit. And FCS keys on a wall-clock DateTime: two version changes inside one clock tick share a stamp; carrying the previous entry's stamp forward (max UtcNow (previous + 1 tick)) would make it strictly increasing per project.
|
🔍 Tooling Safety Check — Affects-Design-Time
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2dd1a17 to
ced4562
Compare
…ce stamps Addresses dotnet#20460 (comment) by proving the specific claim the fix depends on, at the FCS level rather than through FSharpProjectOptionsManager (which the editor test host cannot exercise with a real C# project - see the review's earlier note on that gap). mkCountedCSharpPEReference builds a PEReference backed by an independent CSharpCompilation each time, with a counted getStream (the PE emit / metadata-import entry point DelayedILModuleReader lazily invokes). Two facts: - Reusing a CSharp reference's stamp avoids re-emitting a recreated Compilation: checking the same F# file first against one reference, then against a second reference for a different Compilation but the SAME stamp, leaves the second reference's emit count at 0 - FSharpProjectOptions.AreSameForChecking's structural fallback (both Stamp fields are None here) treats the two ReferencedProjects arrays as equal via PEReference's custom Equals (OutputFile + getStamp()), so the checker's incrementalBuildersCache reuses the existing build and never touches the new reference. - Changing a CSharp reference's stamp does re-emit the new Compilation: the same setup with a different stamp does invoke the second reference's getStream at least once - the control that rules out the first test passing by some unrelated always-cached path. Both verified locally (FSharpSuite.Tests.fsproj, filter-method "*CSharp reference's stamp*"): 2/2 pass. The full MultiProjectTests class (5 facts) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ce stamps Addresses dotnet#20460 (comment) by proving the specific claim the fix depends on, at the FCS level rather than through FSharpProjectOptionsManager (which the editor test host cannot exercise with a real C# project - see the review's earlier note on that gap). mkCountedCSharpPEReference builds a PEReference backed by an independent CSharpCompilation each time, with a counted getStream (the PE emit / metadata-import entry point DelayedILModuleReader lazily invokes). Two facts: - Reusing a CSharp reference's stamp avoids re-emitting a recreated Compilation: checking the same F# file first against one reference, then against a second reference for a different Compilation but the SAME stamp, leaves the second reference's emit count at 0 - FSharpProjectOptions.AreSameForChecking's structural fallback (both Stamp fields are None here) treats the two ReferencedProjects arrays as equal via PEReference's custom Equals (OutputFile + getStamp()), so the checker's incrementalBuildersCache reuses the existing build and never touches the new reference. - Changing a CSharp reference's stamp does re-emit the new Compilation: the same setup with a different stamp does invoke the second reference's getStream at least once - the control that rules out the first test passing by some unrelated always-cached path. Both verified locally (FSharpSuite.Tests.fsproj, filter-method "*CSharp reference's stamp*"): 2/2 pass. The full MultiProjectTests class (5 facts) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3d74593 to
1119a1e
Compare
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ If this fixes an issue or implements an RFC/suggestion, link it (Fixes #... when applicable). Otherwise, give a short management-level summary in simplified technical English: what user scenario improves and what this achieves.
Please apply this PR-description guidance. Remove the implementation inventory already visible in Files, but keep necessary scope, compatibility, and dependency caveats.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ce stamps Addresses dotnet#20460 (comment) by proving the specific claim the fix depends on, at the FCS level rather than through FSharpProjectOptionsManager (which the editor test host cannot exercise with a real C# project - see the review's earlier note on that gap). mkCountedCSharpPEReference builds a PEReference backed by an independent CSharpCompilation each time, with a counted getStream (the PE emit / metadata-import entry point DelayedILModuleReader lazily invokes). Two facts: - Reusing a CSharp reference's stamp avoids re-emitting a recreated Compilation: checking the same F# file first against one reference, then against a second reference for a different Compilation but the SAME stamp, leaves the second reference's emit count at 0 - FSharpProjectOptions.AreSameForChecking's structural fallback (both Stamp fields are None here) treats the two ReferencedProjects arrays as equal via PEReference's custom Equals (OutputFile + getStamp()), so the checker's incrementalBuildersCache reuses the existing build and never touches the new reference. - Changing a CSharp reference's stamp does re-emit the new Compilation: the same setup with a different stamp does invoke the second reference's getStream at least once - the control that rules out the first test passing by some unrelated always-cached path. Both verified locally (FSharpSuite.Tests.fsproj, filter-method "*CSharp reference's stamp*"): 2/2 pass. The full MultiProjectTests class (5 facts) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1119a1e to
69cb60a
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ce stamps Addresses dotnet#20460 (comment) by proving the specific claim the fix depends on, at the FCS level rather than through FSharpProjectOptionsManager (which the editor test host cannot exercise with a real C# project - see the review's earlier note on that gap). mkCountedCSharpPEReference builds a PEReference backed by an independent CSharpCompilation each time, with a counted getStream (the PE emit / metadata-import entry point DelayedILModuleReader lazily invokes). Two facts: - Reusing a CSharp reference's stamp avoids re-emitting a recreated Compilation: checking the same F# file first against one reference, then against a second reference for a different Compilation but the SAME stamp, leaves the second reference's emit count at 0 - FSharpProjectOptions.AreSameForChecking's structural fallback (both Stamp fields are None here) treats the two ReferencedProjects arrays as equal via PEReference's custom Equals (OutputFile + getStamp()), so the checker's incrementalBuildersCache reuses the existing build and never touches the new reference. - Changing a CSharp reference's stamp does re-emit the new Compilation: the same setup with a different stamp does invoke the second reference's getStream at least once - the control that rules out the first test passing by some unrelated always-cached path. Both verified locally (FSharpSuite.Tests.fsproj, filter-method "*CSharp reference's stamp*"): 2/2 pass. The full MultiProjectTests class (5 facts) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
69cb60a to
3951c85
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate correctness issues remain, and coverage does not exercise the manager’s new cache behavior.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Caches in-memory C# PE references by dependent semantic version to reduce redundant metadata emission and F# rechecking.
Changes:
- Adds per-project PE reference caching and cleanup.
- Adds focused reuse and invalidation tests.
- Adds Visual Studio release notes.
| File | Review findings |
|---|---|
vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs |
Four moderate findings (1 vote each): body-only C# edits still invalidate F# checks; same-version cache hits can retain a failed compilation; DateTime.UtcNow may produce equal stamps for replacements; and output-path changes are not included in the cache identity. |
tests/fsharp/Compiler/Service/MultiProjectTests.fs |
Nit (2 votes): tests exercise FCS directly rather than FSharpProjectOptionsManager, so the new per-project cache behavior is not covered. |
docs/release-notes/.VisualStudio/18.vNext.md |
Nit (4 votes): three duplicate release-note bullets should be reduced to one, preferably the entry with the PR link. |
Picking each pull request's release note again after it was reworded or given its link left the earlier copies in docs/release-notes/.VisualStudio/18.vNext.md, which merges as a union. Each pull request keeps the bullet its own branch, or main, has now. - The copy without the PR link: dotnet#20409, dotnet#20457, dotnet#20460, dotnet#20482, dotnet#20519. - The second of two identical copies: dotnet#20274, dotnet#20455, dotnet#20468, dotnet#20470, dotnet#20482, dotnet#20499, dotnet#20503, dotnet#20534 and the Navigate To ordering bullet. - The wording a later one replaced: dotnet#20126, dotnet#20274, dotnet#20457, dotnet#20460. The bullets of main are each there once and in main's order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Applies what the pull request gained after cb99b2f. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ce stamps Addresses dotnet#20460 (comment) by proving the specific claim the fix depends on, at the FCS level rather than through FSharpProjectOptionsManager (which the editor test host cannot exercise with a real C# project - see the review's earlier note on that gap). mkCountedCSharpPEReference builds a PEReference backed by an independent CSharpCompilation each time, with a counted getStream (the PE emit / metadata-import entry point DelayedILModuleReader lazily invokes). Two facts: - Reusing a CSharp reference's stamp avoids re-emitting a recreated Compilation: checking the same F# file first against one reference, then against a second reference for a different Compilation but the SAME stamp, leaves the second reference's emit count at 0 - FSharpProjectOptions.AreSameForChecking's structural fallback (both Stamp fields are None here) treats the two ReferencedProjects arrays as equal via PEReference's custom Equals (OutputFile + getStamp()), so the checker's incrementalBuildersCache reuses the existing build and never touches the new reference. - Changing a CSharp reference's stamp does re-emit the new Compilation: the same setup with a different stamp does invoke the second reference's getStream at least once - the control that rules out the first test passing by some unrelated always-cached path. Both verified locally (FSharpSuite.Tests.fsproj, filter-method "*CSharp reference's stamp*"): 2/2 pass. The full MultiProjectTests class (5 facts) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Picking each pull request's release note again after it was reworded or given its link left the earlier copies in docs/release-notes/.VisualStudio/18.vNext.md, which merges as a union. Each pull request keeps the bullet its own branch, or main, has now. - The copy without the PR link: dotnet#20409, dotnet#20457, dotnet#20460, dotnet#20482, dotnet#20519. - The second of two identical copies: dotnet#20274, dotnet#20455, dotnet#20468, dotnet#20470, dotnet#20482, dotnet#20499, dotnet#20503, dotnet#20534 and the Navigate To ordering bullet. - The wording a later one replaced: dotnet#20126, dotnet#20274, dotnet#20457, dotnet#20460. The bullets of main are each there once and in main's order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Applies what the pull request gained after cb99b2f. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ce stamps Addresses dotnet#20460 (comment) by proving the specific claim the fix depends on, at the FCS level rather than through FSharpProjectOptionsManager (which the editor test host cannot exercise with a real C# project - see the review's earlier note on that gap). mkCountedCSharpPEReference builds a PEReference backed by an independent CSharpCompilation each time, with a counted getStream (the PE emit / metadata-import entry point DelayedILModuleReader lazily invokes). Two facts: - Reusing a CSharp reference's stamp avoids re-emitting a recreated Compilation: checking the same F# file first against one reference, then against a second reference for a different Compilation but the SAME stamp, leaves the second reference's emit count at 0 - FSharpProjectOptions.AreSameForChecking's structural fallback (both Stamp fields are None here) treats the two ReferencedProjects arrays as equal via PEReference's custom Equals (OutputFile + getStamp()), so the checker's incrementalBuildersCache reuses the existing build and never touches the new reference. - Changing a CSharp reference's stamp does re-emit the new Compilation: the same setup with a different stamp does invoke the second reference's getStream at least once - the control that rules out the first test passing by some unrelated always-cached path. Both verified locally (FSharpSuite.Tests.fsproj, filter-method "*CSharp reference's stamp*"): 2/2 pass. The full MultiProjectTests class (5 facts) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
110485f to
9c8fc1d
Compare
…n holds createPEReference keyed the reference on the identity of the Roslyn Compilation and stamped it with DateTime.UtcNow at creation. Roslyn recreates Compilation instances freely - on every solution fork, and under memory pressure because it holds the final compilation weakly - so a dependent F# project kept receiving a reference with a fresh stamp. That stamp feeds the project snapshot's base version, so BootstrapInfo was invalidated, every reference re-imported and every file re-checked, even though the referenced assembly's metadata had not changed. The reference is now cached per referenced project and reused while the project's dependent semantic version is unchanged; a newer Compilation only refreshes the source the delayed reader emits from. Metadata-only emit depends on the public surface, which is what that version tracks, so C# edits below the declaration level no longer invalidate F# checking either. The ConditionalWeakTable is gone with it: its value pinned the compilation until the first emit, which kept the key alive, so entries were never collected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mpilation with <see cref> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dropped - Look the reference up by dependent semantic version before asking for a Compilation, so a cache hit no longer rebuilds and re-pins one Roslyn dropped under memory pressure (matching SolutionCompilationState.SkeletonReferenceCache). This removes Refresh, and with it the reactor/emit-thread race that could re-pin a compilation after a successful emit, so no lock is needed; and! is gone with it. - TryGetCompilation prefers the pinned compilation and falls back to the weak reference, instead of assuming the two agree. - Split tryGetPEReference from createPEReference and rename createNewPEReference to buildPEReference. - ClearOptions: hoist the lastSuccessfulCompilations and peReferences removals out of the cache.TryRemove arm - cache holds F# project ids, both dictionaries hold referenced C# ids, so the removals never ran. Drop their ClearSingleFileOptionsCache twins, where the id is the miscellaneous-files project and can never be a key. - Drop comments the code already states, and trim the release note to the user-visible effect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ce stamps Addresses dotnet#20460 (comment) by proving the specific claim the fix depends on, at the FCS level rather than through FSharpProjectOptionsManager (which the editor test host cannot exercise with a real C# project - see the review's earlier note on that gap). mkCountedCSharpPEReference builds a PEReference backed by an independent CSharpCompilation each time, with a counted getStream (the PE emit / metadata-import entry point DelayedILModuleReader lazily invokes). Two facts: - Reusing a CSharp reference's stamp avoids re-emitting a recreated Compilation: checking the same F# file first against one reference, then against a second reference for a different Compilation but the SAME stamp, leaves the second reference's emit count at 0 - FSharpProjectOptions.AreSameForChecking's structural fallback (both Stamp fields are None here) treats the two ReferencedProjects arrays as equal via PEReference's custom Equals (OutputFile + getStamp()), so the checker's incrementalBuildersCache reuses the existing build and never touches the new reference. - Changing a CSharp reference's stamp does re-emit the new Compilation: the same setup with a different stamp does invoke the second reference's getStream at least once - the control that rules out the first test passing by some unrelated always-cached path. Both verified locally (FSharpSuite.Tests.fsproj, filter-method "*CSharp reference's stamp*"): 2/2 pass. The full MultiProjectTests class (5 facts) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Linking the note added a copy of it instead of editing the first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The long version was replaced rather than removed when the note was cut down to the user-visible effect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A source generator can turn an edit below the declaration level into different public declarations, and such an edit leaves the dependent semantic version of the project, and of every project that depends on it, standing. The cached PE reference then kept the old declarations: after a body edit that renamed a generated member, F# reported FS0039 for the new name. A referenced project that has analyzer references, or depends on a project that has them, is now versioned by its dependent version, which every edit moves. Analyzer references stand in for generators because only Roslyn's out-of-process host knows which of them carry one; asking in Visual Studio would load the generators there. A project without analyzer references keeps the dependent semantic version. The test needs C# projects, so the editor tests reference Microsoft.CodeAnalysis.CSharp.Workspaces and host them in a plain workspace. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… note F# checks still re-run after a C# edit that keeps the reference: the F# project options are stamped with the dependent version. The note also names the narrower rule for a project that may run source generators. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The editor test host served language services for F# only, so a C# project could not be added to its workspace. It now creates them for any language from the same export provider, imports the C# workspace parts, and accepts .cs documents. These are the host changes of f497098 in dotnet#20463, without its helpers, so that the two pull requests merge without a conflict in the host. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The tests ask the real FSharpProjectOptionsManager for the options of an F# project that references a C# project in the editor test workspace, and look at the PE reference the options hold: - an edit that leaves the C# declarations alone keeps it, although the options are computed again; - an edit of the declarations replaces it, and the replacement is kept in turn; - a C# project with an analyzer reference gets a new one on every edit; - removing the C# project, and clearing all caches, drop it. The removal test brings the very same project back, in the very same state, so that an entry which survived the removal would be reused. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The helpers blocked on Async.RunSynchronously, Task.Wait and Task.Result. They return tasks now, and the tests are tasks that await them, as InlineRenameTests does. peReferenceOf filters through Seq, so no intermediate array is built. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
9c8fc1d to
cca0df7
Compare
The comments are down to the reasons the code cannot say: the field that is held strongly until the first emit is named pinnedUntilEmitted instead of carrying a comment, the test bindings are named for what they are, and the review pointer and the restating comments are gone. The two editor test files share the C# sources and the analyzer reference instead of declaring each their own. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Applied the NoBloat/CodeCompaction request of the 10-13 review in 881f5b0, on top of the branch rebased onto main 291f2bf.
Diff against main: +495/−66 (was +530/−70). The |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Roslyn returns a new
Compilationfor a referenced C# project on every solution fork, and each one got a reference with a fresh stamp, so FCS emitted a PE image and built a metadata reader for it again however little the project had changed. The reference is now kept per C# project and reused while the project's dependent semantic version is unchanged (its dependent version for a project with analyzer references, since a source generator can turn any edit into different declarations); an edit of the declarations replaces it, and removing the project drops it.That is all it changes: how often F# files are checked again after a C# edit follows the options of the F# project, which a C# edit still replaces.
🤖 Generated with Claude Code